-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updated versions of DESY P11 HO (PR 800, 854) #856
Conversation
Hi @agruzinov, all looks fine for me. Thanks for the effort. |
sys.path.append("/opt/dectris/albula/4.0/python/") | ||
|
||
from mxcubecore.BaseHardwareObjects import HardwareObject | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of this the AlbulaView, as I understand it it starts Albula on the same machine as MXCuBE is running and gets the images from the detector zero-mq stream ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it is not completely implemented. Also in the python 3.11 this line leads to the segfault anyway. This will require a bit more work yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, i see. Did you also foresee to display a certain image on demand ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally it is working right now with in-house software as a standard viewer (made by Jan). It shows the monitor interfaces during data collection and than changes to the recently collected file and shows it afterwards, so one can scroll back if needed. I was trying to understand whether the Adxv can show anything during data collection, but before hdf5 is saved, there is no way I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not sure. I don't think so
# self.omega_mv(init_pos, self.default_speed) | ||
self.collect_std_collection(start_at, stop_angle) | ||
|
||
# This part goes to standard collection. Otherwise it produces phantom openings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general good practice is to remove commented code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
@@ -236,15 +267,24 @@ def input_from_params(self, data_collection, char_params): | |||
acquisition_parameters = data_collection.acquisitions[0].acquisition_parameters | |||
path_template = data_collection.acquisitions[0].path_template | |||
|
|||
# Make sure there is a proper path conversion between different mount points | |||
print( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using log instead of print ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
Looks good to me, just a few comments. |
@@ -340,11 +340,11 @@ def collect_characterisation( | |||
self.omega_mv(start_angle, self.default_speed) | |||
|
|||
for img_no in range(nimages): | |||
print("collecting image %s" % img_no) | |||
logging.info("collecting image %s" % img_no) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess here in this case it would be better to use self.log
instead of logging
. And maybe also debug
instead of info
. So maybe self.log.debug
.
[Side note: Logging is all over the place in mxcube, we should seriously look at it at some point. Maybe set up a clear policy, and if possible enforce it with some linting. Feels like everyone writes whatever they feel like on the moment, no one looks at the existing code base or at the docs.]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would indeed be very nice to cleanup the logging, then I think we should be bit more liberal for site specific files like this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment. I still need all these logs/debugs because the system is still in active change. Once it is stabilised, the excessive logs should indeed go. But for now, it is hard to debug things without them.
The linting step is failing, Ill merge when its fixed :). Thanks ! |
black and autopep8 are ran on the code but it is not yet happy still... What could it be? |
Autoflake: https://github.com/mxcube/mxcubecore/actions/runs/8063131751/job/22024333705?pr=856#step:6:29 You should be able to run |
@agruzinov do the follwing in your shell |
Great ! 👍 |
As discussed in PR 800/854 here is the DESY P11 HO objects cumulative PR.